Hi there 👋, @DryRunSecurity here, below is a summary of our analysis and findings.

Powered by DryRun Security

@lme-nca I've re-kicked the tests but even if they all pass, you'll need to fix the Ruff linter issue before we can start the approval process.

@lme-nca this looks like the proposal is to use a generic OID backend with some key cloak references mixed in. If we are to generalize on OIDC, I think it should be done all the way.

This proposal raises a question: do we want to support a generic OIDC mechanism? It seems like OAuth is more widely used at this point, and would add more value. @mtesauro what do you think?

Hi @Maffooch & @mtesauro,

Thanks for looking at the pull request.

Currently defectdojo is kind of already using OIDC not just Oauth in your keycloak implementation, see defectdojos own documentation on how to set up the client in Keycloak: https://defectdojo.github.io/django-DefectDojo/integrations/social-authentication/#configure-keycloak Keycloak always provides oidc on top of Oauth, like just about any other authentication provider. Furthermore, the current keycloak provider is unusable as it hardcodes the public key, making key rotation by the provider impossible.....

Another example is Defectdojo's "Auth0" integration, which is based on https://github.com/python-social-auth/social-core/blob/master/social_core/backends/auth0.py which also reads the "id_token" value, which is a OIDC only concept, even though the class only inherits from the "BaseOAuth2" baseclass. It should probably instead also inherit from https://github.com/python-social-auth/social-core/blob/master/social_core/backends/open_id_connect.py The same goes for OCTA which uses the OIDC well known endpoint: https://github.com/python-social-auth/social-core/blob/master/social_core/backends/okta.py

The fact is that nearly all Authentication providers use a mix of Oauth and OIDC protocol elements to provider their services and a lot of the providers in https://github.com/python-social-auth/social-core/tree/master are not distinguishing this correctly.

@Maffooch

The problem with generalizing to OID is that this pull request relies on keycloak specific groups and mapper mechanisms to sync groups from keycloak to defectdojo (i.e. it requires the use of https://www.keycloak.org/docs-api/21.1.2/javadocs/org/keycloak/protocol/oidc/mappers/GroupMembershipMapper.html ). Now other providers probably have very similar mechanisms but if they work the same depends heavily on their configuration..... Therefore I would suggest that from a user point of view this is all Keycloak specific (that this uses the generic OIDC connect backend is irrelevant).

Therefore I propose to keep the configuration from Defectdojo side "keycloak" specific, while under water using the OIDC provider of social core to properly integrate with keycloak.

alternatively: the authentication part of keycloak, could be made generic "OIDC" and the configurations of the group syncing would then be "keycloak" specific named configuration settings. Then the documentation would reflect that the usage of keycloak as an authentication provider would just be 1 example of using a OIDC compatible provider with the option to enable keycloak specific group syncing....

@lme-nca I'm not opposed to this change in the keycloak integration - I get the points you're making about these being Keycloak-specific groups.

However, to move forward, the tests would have to be green both Ruff and the various unit-tests.

@lme-nca I'm not opposed to this change in the keycloak integration - I get the points you're making about these being Keycloak-specific groups.

However, to move forward, the tests would have to be green both Ruff and the various unit-tests.

@mtesauro Ok the tests are green now, I also made a note in the documentation (docs/content/en/integrations/social-authentication.md) as this should still be updated. Let me know if you agree with the technical part of the Pull request and then ill update the documentation to match the new process.

This pull request has conflicts, please resolve those before we can evaluate the pull request.

Conflicts have been resolved. A maintainer will review the pull request shortly.

@mtesauro @Maffooch All tests pass, documentation has been updated. Would really like to merge this to avoid further conflicts down the road.....

This pull request has conflicts, please resolve those before we can evaluate the pull request.

Thank you for contributing to DefectDojo! We have a mantra around here with regards to merging a pull request: “A no is just for today, but a yes is forever”. With that said, we must say no to this PR for today because it has gone stale

@Maffooch @mtesauro : So we are just happy to have a broken and insecure Keycloak integration in Defectdojo ??? I really would like to fix it, but for this I need some kind of Project support to get this pull request approved ?

@lme-nca

So we are just happy to have a broken and insecure Keycloak integration in Defectdojo ???

No, of course not. I don't have Keycloak nor any experience with it so I'm not in a place to say if there are deficiencies in the support for Keycloak we get from the social-auth Python module we're using.

I really would like to fix it, but for this I need some kind of Project support to get this pull request approved ?

We'd really like someone in the community with access to Keycloak to help maintain that SSO provider. @Maffooch wrote "because it has gone stale" which it had when he closed this PR.

One of the things we're starting to use to keep the PR list clean is to look at the last time an update happened to a PR with a conflict. There's been no updates to this PR since a merge conflict happened back on June 3rd. I think if you ask most code maintainers if a PR which a merge conflict and no updates for ~4 months was stale, they'd say yes. That's what we did here.

You're welcome to re-open this PR or start a new one - @Maffooch did say "A no is just for today, but a yes is forever" which seems fair for a feature that requires access to a 3rd party system which is non-trivial to setup and maintain.

You might also look at this currently open PR - #10614 that is adding generic OIDC login which might accomplish what you were looking for here.

Cheers!

@mtesauro the conflict happens because of the HASH check (or in my case database change) which happens every single time another change gets merged (that changes the hash or db)... it would be easier to review the code, approve it and then when we are ready to merge fix this last "conflict" which is not a real conflict but an artificial barrier introduced that is actively stopping people from contributing as these pull request simply get ignored/not reviewed afterwards...

(look here, he also went trough the useless effort: f97900e ) sadly since then there is a new conflict..... and now he has to jump through hoops again....

Honestly, there is a great community of people who would like to contribute, but everyones time is valuable and this process is disrespectful of the time that people put into these pull requests.

(EDIT)
I see that you recognise this too now: #11299

@lme-nca Yes, you found what I was going to point out.

We're aware that an effort to try to keep people deploying and using DefectDojo in a incorrect way has had the side effect of making it harder for the community to make contributions since they are frequently getting tripped up by that sha256sum. And, we're working through the best way to remove that contribution burden. If you're still interested in restarting this work and want to wait till that's merged, I completely understand that position.

BTW, I don't believe the script that Maffooch used to find the stale PRs was clever enough to make the distinction between sha256 conflicts and other merge conflicts though I've not seen it myself. TBH, I'm not sure if GH's API gives you those detail as I've never looked at that part of their API. Once #11299 is merged, that lack of distinction should no longer matter.